Repository navigation
fix!: exclude downtime from crawler_runtime after a resume - #2204
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2 #2204 +/- ##
=====================================
Coverage ? 94.18%
=====================================
Files ? 187
Lines ? 13823
Branches ? 0
=====================================
Hits ? 13019
Misses ? 804
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Waiting for V2 as this might be breaking change and will inlcude removal of deprecated fields |
4b9f3e5 to
8af9ee3
Compare
de270b1 to
7ca479e
Compare
…r resurrection The `crawler_runtime` statistic no longer counts the downtime between the runs of a resumed crawler, and it accumulates correctly across any number of persist/resume cycles: - The first periodic statistics log reports exactly the runtime accumulated so far: zero for a fresh start, the previously persisted total for a resumed run. - The accumulated runtime is stored in the new `accumulated_runtime` field, persisted under `crawlerRuntimeMillis`. Every persist writes the live total, so a state persisted mid-run (migration, abort) carries the runtime up to that moment and a resumed run continues from the last checkpoint. - The timestamps of the previous run are cleared before the new run starts being measured, so they cannot leak the downtime into the runtime, and a backwards clock jump during a run cannot push the runtime below the accumulated total. BREAKING CHANGE: `StatisticsState.crawler_runtime_for_serialization` is removed; the `crawler_runtime` property is backed by the new `accumulated_runtime` field. `crawlerRuntimeMillis` is serialized as a number of milliseconds instead of an ISO 8601 duration string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VMHwDx87jV4rnu5LkbpNxy
7ca479e to
fbab2a8
Compare
apify-factory
left a comment
There was a problem hiding this comment.
Looks good, just one note.
Outside the diff:
src/crawlee/statistics/_models.py: Note:crawlerRuntimeMillisis serialized as the total (including the last run's segment) but validated back intoaccumulated_runtimealongside the unchangedcrawlerLastStartTimestamp/crawlerFinishedAt, soStatisticsState.model_validate(state.model_dump(by_alias=True)).crawler_runtimecounts the last run twice. OnlyStatistics.__aenter__clears the timestamps, so any consumer that parses a persisted statistics record withStatisticsStategets an inflated runtime.
🏭 Factory review — ℹ️ not auto-approved
This change was reviewed but cannot be auto-approved: a gate failed, so the review cannot end in an approval.
| check | result |
|---|---|
| Static gates | ❌ 13/16 — base-branch, max-lines, pr-title |
| Review findings | ❌ not clean |
| Reviewer escalation | ❌ Changes the persisted statistics format (crawlerRuntimeMillis type and meaning) and removes a public model field, a breaking public contract that downstream consumers like the Apify SDK may read. |
Juror 1 — claude-sonnet-5-5, adversarial |
⏭️ skipped — a gate failed |
Why it was not auto-approved
Failed gates:
base-branch: base is v2, required mastermax-lines: 217 changed lines (+194/-23), limit 200pr-title: breaking changes are never auto-approved
The comments above are the review itself; this section only explains the approval decision. Push a change and request @apify-factory again to re-run it, or ask a human to review.
Reviewed fc873682f · workflow run
That is not how the |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VMHwDx87jV4rnu5LkbpNxy
|
The title is 84 characters long. Could we use something more concise? E.g.:
|
Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>
Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>
Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>
vdusek
left a comment
There was a problem hiding this comment.
Last 3 nits, otherwise LGTM
Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>
crawler_runtimeincluding the downtime between the runs (e.g.38.01sfor a ~0.5 s run); later logs corrected themselves. Now the first log reports exactly the runtime accumulated so far (0 for a fresh run).StatisticsState.accumulated_runtimenow stores the runtime of the previous runs; the read-onlycrawler_runtimeproperty adds the run in progress, and every persist checkpoints the live total.crawler_runtime_for_serializationis removed, andcrawlerRuntimeMillisis persisted as a number of milliseconds instead of an ISO 8601 duration string. Documented indocs/upgrading/upgrading_to_v2.md.StatisticsState.crawler_runtimesetter #1567.